Use rust-native error for RemoveIdentityEquiv - #16195
Conversation
raynelfss
added
on hold
|
One or more of the following people are relevant to this code:
|
Coverage Report for CI Build 26459066270Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.003%) to 87.513%Details
Uncovered Changes
Coverage Regressions17 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
gadial
left a comment
gadial
left a comment
There was a problem hiding this comment.
Looks good to me.
My only question is whether the .map_err(RemoveIdentityEquivError::PyPauliRotationTraceAndDim)?; can be avoided by more implicit conversion, and whether it should be avoided like this. But it's a very minor concern.
The following commits modify `RemoveIdentityEquiv` to use rust-native errors throughout its pipeline. In this PR we introduce the error enumeration `RemoveIdentityEquivEerror` which only has two variants, one for `DAGCircuitError` and the other is for a specific case involving `PauliEvolution` gates that only exist in Python currently. These changes mostly depend on Qiskit#16134.
e3b94ca to
215c313
Compare
| #[derive(Debug, thiserror::Error)] | ||
| pub enum RemoveIdentityEquivError { | ||
| #[error(transparent)] | ||
| DAGCircuit(#[from] DAGError), |
There was a problem hiding this comment.
Where does the run_remove_identity_equiv function return a DAGError? Isn't it all PyPauliRotationTraceAndDim?
There was a problem hiding this comment.
Hmmm... I think I was under the asusmption it did but it actually doesn't at all. In that case, since the only error is a Python error, should I replace it with a unique error type (a la #16133)? It sounds like a better option (we'd have to use one fixed message for the error though), but at this point I'm not sure it is much better than having it return PyResult in the end.
There was a problem hiding this comment.
I looked at this further and the thing is I'm not sure what type of exception might be returned by pauli_rotation_trace_and_dimension. It's not very clear from the code itself but I can assume once PauliEvolutionGate is added to Rust (if planned) this method may become infallible. But other than that, the only solution would be to create an error that just wraps a PyErr. However, I feel that might be a bit redundant at the end of the day.
I could also make a default rust error with a generic message that maps down to a python error but I feel it might be misleading to a user trying to debug why their function call failed. Not to mention it would lose all context of its origin.
6 participants
The following commits modify
RemoveIdentityEquivto use rust-native errors throughout its pipeline.In this PR we introduce the error enumeration
RemoveIdentityEquivEerrorwhich only has two variants, one forDAGCircuitErrorand the other is for a specific case involvingPauliEvolutiongates that only exist in Python currently.Use this to visualize the actual diff
Pre-requisites
DAGCircuit#16134AI/LLM disclosure